-
Notifications
You must be signed in to change notification settings - Fork 117
Enable exit test value capturing #1165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This PR enables the value capture subfeature of exit tests and adds documentation for it. > [!WARNING] > Do not merge this PR unless/until [ST-NNNN](swiftlang/swift-evolution#2886) has been approved.
@swift-ci test |
@swift-ci test |
@swift-ci test |
@swift-ci test |
Waiting for #1208 before merging as it has ABI impact. |
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few suggestions but looks good mostly.
} | ||
``` | ||
|
||
- Note: If you use this macro with a Swift compiler version lower than 6.3, it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest "the macro" instead of "this macro" here, to be clear you're talking about that one rather than @Test
which the snippet also uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This language is the same as what we did for #expect(throws:)
. Did you want to change it there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to say "the expect(processExitsWith:)
macro", not sure why that didn't come out. Anyway, if we're writing in the reference doc for a macro, "this macro" clearly refers to the one we're describing. In an article, and particularly one where we've just used two different macros in one example, there's no "this" binding.
If a captured value is an argument to the current function or is `self`, its | ||
type is inferred at compile time. Otherwise, explicitly specify the type of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"…the compiler infers the value's type." to avoid passive voice.
Every value you capture in an exit test must conform to [`Sendable`](https://developer.apple.com/documentation/swift/sendable) | ||
and [`Codable`](https://developer.apple.com/documentation/swift/codable). Each | ||
value is encoded by the parent process using [`encode(to:)`](https://developer.apple.com/documentation/swift/encodable/encode(to:)) | ||
and is decoded by the child process [`init(from:)`](https://developer.apple.com/documentation/swift/decodable/init(from:)) | ||
before being passed to the exit test body. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passive voice here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent process encodes each value using
encode(to:)
and the child process decodes each value usinginit(from:)
before passing them to the exit test body.
Feels worse somehow. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
The testing library encodes each value using
encode(to:)
in the parent process, and decodes it usinginit(from:)
in the child process, before passing it to the exit test body.
Because it's the testing library that actually does all of this.
This PR enables the value capture subfeature of exit tests and adds documentation for it.
Resolves #1157.
Checklist: